Size Change: -858 B (-0.05%)

Total Size: 1.77 MB

_{compressed-size-action}

Flaky tests detected in d357926.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10420898477
📝 Reported issues:

• [Flaky Test] should not render Block Settings sections #64387 in /test/e2e/specs/widgets/customizing-widgets.spec.js

Always good to press the issue.

I have the experiment enabled, at the moment, and I think there are a couple of other things we can try first. For further context, when I have no bindings registered, and no plugins active, I don't see the panel:

That's great, it's how it should be.

As soon as I enable Jetpack, it shows up, like this:

This is because Jetpack registers a _jetpack_memberships_contains_paid_content which gets picked up here. No doubt other plugins register similar pieces, that will make it show up. In fact it seems likely that many of the most commonly used plugins will cause this panel to appear. It's not clear to me if we can delineate between this metadata, or even if we want to (probably not), but to me it suggests that we should revisit the empty state of the panel. Quick mockup:

Changes here:

• Entire panel is moved to the bottom, right above "Advanced".
• Help text, "Attributes connected to various sources", is shown only when you've set an attribute, and is $helptext-font-size (i.e. 12px)
• value in the ItemGroup is similarly help-text font size.

Both of these would reduce the footprint of the panel when you're not using it, to the point that it might be fine to have there, without explicitly enabling or disabling it.

Which is also to say, we might still add an option to disable it, though perhaps it should default to on?

Joen's proposal makes plenty of sense to me. I'd go one step further and don't display the help text at all, no matter if there are attributes or not. We don't explain what other sections are for so this feels a bit inconsistent. Also, the text itself isn't very helpful because it doesn't say what attributes or sources are, and given that it's only shown if there's at least one connection, the user is likely familiar with the concept.

Figma, btw, if you want to tinker! Forgot to add that in my previous comment.

Yes, the empty panel is something we wanted to improve as well. And the suggestions made make sense to me. Thanks for sharing them 🙂 On the other hand, I believe we can work on that on another pull request.

For further context, when I have no bindings registered, and no plugins active, I don't see the panel

If that's the case that might be a "bug". When you have the experiment enabled, the idea is to always show the empty panel because users should be able to create bindings for an empty paragraph, for example.

Both of these would reduce the footprint of the panel when you're not using it, to the point that it might be fine to have there, without explicitly enabling or disabling it.

Which is also to say, we might still add an option to disable it, though perhaps it should default to on?

The main problem with having the default to on is the paragraph block. Right now, the paragraph block doesn't have any "Settings", so when you enter a new paragraph, it just shows the "Styles".

If we introduce a new setting like in this case, it creates a new tab for "Settings", which is selected by default.

That means that it changes the workflows of any paragraph.

You can find more context in this comment in the UI concerns section: link.

Help text, "Attributes connected to various sources", is shown only when you've set an attribute

I don't like the current empty panel and I agree that "Attributes connected to various sources" sounds confusing. On the other hand, I feel just having "Attributes" isn't clear enough for users. We discussed in the past having Bindings or Connections but the feedback was it is too technical. Other possibilities could be "Dynamic Data" or "Custom Fields" if we decide to keep this UI just for custom fields and not other sources.

If that's the case that might be a "bug". When you have the experiment enabled, the idea is to always show the empty panel because users should be able to create bindings for an empty paragraph, for example.

Right, good point.

The main problem with having the default to on is the paragraph block. Right now, the paragraph block doesn't have any "Settings", so when you enter a new paragraph, it just shows the "Styles".
If we introduce a new setting like in this case, it creates a new tab for "Settings", which is selected by default.

That does seem to be the key issue to solve indeed.

Can we make an exception for this panel, just like there is an exception for "Advanced", so that it doesn't split the inspector?

It would still, like the Advanced panel, show up only on the Settings tab if that tab is present.

Can we make an exception for this panel, just like there is an exception for "Advanced", so that it doesn't split the inspector?

That's something we considered and I believe I actually have a local branch with that change working. However, it would mean having this panel in the "Styles" tab, which doesn't seem correct and it is some feedback we got if I remember correctly.

However, it would mean having this panel in the "Styles" tab, which doesn't seem correct and it is some feedback we got if I remember correctly.

Are you sure? The "Advanced" panel doesn't show up in the Styles tab, only in the Settings tab, yet it doesn't cause the Paragraph to have the split tabs.

I might be confusing what is "Styles" tab and what is "Settings" tab. For the paragraph this is how it looks like:

I assumed those were just styles and that's why I called it "Styles" tab. Although it's true there is no mention for "Styles". I'm happy to introduce it above the "Advanced" section.

Ah, in that case, the "Advanced" panel is exempt from the rules. For any inspector that splits in Settings & Styles, the "Advanced" panel will show up on the "Settings" tab. But it doesn't, as you can see, cause the Paragraph to split into those two. And similarly I could see the same behavior applied to Attributes.

I have just pushed some changes to make it work that way:

Image with URL connected	Empty paragraph

I agree that we need to revisit the help text and probably remove it when there are no bindings. I still believe "Attributes" is not clear enough. What would you think about this:

• Remove the help text when there are no bindings.
• Rename it to "Dynamic Data" or "Connections".
• Change the text when bindings are created to something like "Connected to custom fields or other dynamic data".
• Adapt the font-size as suggested in your comment.

Remove the help text when there are no bindings.

Yes.

Rename it to "Dynamic Data" or "Connections".

I quite like "Attributes", I think we should keep it for the time being.

Change the text when bindings are created to something like "Connected to custom fields or other dynamic data".

I'm personally fine with help text being present when there are bindings, and I'm flexible on the verbiage. Though I do like that the current help text fits on one line. If we do have to go to two lines, need to make sure it doesn't wrap in a way that creates a typographic widow. That should be automatically handled due to text-wrap: pretty;, but mentioning it to make sure.

I have just pushed those changes to the help text.

It doesn't appear when there are no bindings

It shows when there are bindings with a different text

I believe this is ready for review. Now it adds a way to enable/disable this UI and it fixes the issue of interfering with the paragraph block.

I guess there are two questions:

• Is it okay to use "Preferences" to enable/disable the bindings creation?
• Is the UX good enough to remove the experiment?

In my opinion, it is fine for this first iteration and we can keep working on top of it.

I'm gonna be on holiday next week, feel free to push any required changes.

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @jarekmorawski.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Unlinked contributors: jarekmorawski.

Co-authored-by: SantosGuillamot <[email protected]>
Co-authored-by: gziolo <[email protected]>
Co-authored-by: artemiomorales <[email protected]>
Co-authored-by: jasmussen <[email protected]>
Co-authored-by: bacoords <[email protected]>
Co-authored-by: cbravobernal <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

This is because Jetpack registers a _jetpack_memberships_contains_paid_content which gets picked up here.

Should post meta that starts with an underscore be exposed by the Block Bindings API?

It's a precedent that fields starting with an underscore are "hidden" and don't show up in the regular Custom Fields UI. Feels like they shouldn't show up in Bindings either.

@SantosGuillamot

https://developer.wordpress.org/reference/functions/add_post_meta/#comment-467

Should post meta that starts with an underscore be exposed by the Block Bindings API?

Right now, it shows only the fields that are explicitly marked as public to be exposed in the REST API. I assumed that was enough, but I'm happy to hide the ones starting with underscore if that's expected. I don't have enough context to say if that's the case.

The preference can be added later if necessary. One thing to keep in mind if we go with the editor preference, then if it gets set on the server to false, then there should be no way to enable it on the client. So, there might be little incentive to control it for the client according to user preference. In case, the settings is the way to go the in WP core it can be placed somewhere in get_block_editor_settings function:

https://github.com/WordPress/wordpress-develop/blob/e569967ee45e23186f6aa7f479fcb48a7511436b/src/wp-includes/block-editor.php#L487

I was surprised to learn that canLockBlocks is always set to true and it happens on the server here:

Okay, trying to summarize what this means:

• I'll add a canUpdateBlockBindings editor settings that by default is true only for admins.
• I'll remove the preference added in the UI in this pull request for now.
• By default, admin users will be able to create/modify bindings.
• By default, other roles won't be able to create/modify bindings BUT they will see the "read-only" panel when there are existing bindings.
• Developers can add/remove permissions to create/modify bindings through a filter modifying the canUpdateBlockBindings editor setting.
• It won't be possible to completely hide the "read-only" panel, at least for now.

Does this make sense?

Okay, trying to summarize what this means:

* I'll add a `canUpdateBlockBindings` editor settings that by default is `true` only for admins.

* I'll remove the preference added in the UI in this pull request for now.

* By default, `admin` users will be able to create/modify bindings.

* By default, other roles won't be able to create/modify bindings BUT they will see the "read-only" panel when there are existing bindings.

* Developers can add/remove permissions to create/modify bindings through a filter modifying the `canUpdateBlockBindings` editor setting.

* It won't be possible to completely hide the "read-only" panel, at least for now.

Does this make sense?

Yep, still I don't know what to say about editor or authors with their own content. Let's explore later.

I've made the mentioned changes in the latest commits. With that, this pull request covers:

• Create a new "bindings" group that is placed on top of the Advanced settings. This solves the problem of creating a new "Settings" tab for blocks like paragraphs or headings.
• It adds a new editor setting canUpdateBlockBindings to only allow admin users to create and modify connections. This can be change with the block_editor_settings_all filter.
• It removes the experimental flag.
• It polishes the UI and some help texts.

I'll work on a backport in core, and I'll update the description to reflect these changes, but everything should be working now.

Help text, "Attributes connected to various sources", is shown only when you've set an attribute, and is $helptext-font-size (i.e. 12px)

This part from the #64570 (comment) left by @jasmussen wasn't addressed. Have you decided to keep the message? In my opinion, it doesn't bring too much value when no connections are established.

I do think we should add additional styling for the non-admin read only state, like setting the attribute name to gray, to signal that the the UI there has been disabled. Otherwise, the fact that it's disabled could be interpreted as a mistake.

I agree the "read-only" panel could be probably improved. On the other hand, it isn't still clear to me how. I'd vote for creating a separate discussion for that and make the improvements in a follow-up PR. In the end, the "read-only" panel hasn't changed in this PR, the styling is the same that we have right now in trunk without the experiment enabled.

This part from the #64570 (comment) left by @jasmussen wasn't addressed. Have you decided to keep the message? In my opinion, it doesn't bring too much value when no connections are established.

Mmm, it is supposed to be addressed. I shared how it was working in this comment. I have just checked the branch and it is working as expected for me.

Did you see anything different or should I just update the description?

Mmm, it is supposed to be addressed. I shared how it was working in #64570 (comment). I have just checked the branch and it is working as expected for me.

The description and screenshot in the comments are misleading. I couldn't test it using Playground for some reason for the case with no connections. I don't have any more comments then 👍🏻

I have just updated the screenshots in the description. I hope that helps clarifying the current status.

I added a new canUpdateBlockBindings editor setting that it is true for admin users by default.

Just wanted to say how much we appreciate this inclusion. 🙏 (We run MultiSites and our Site Administrators should absolutely not be able to do this kind of stuff. The ability for us to remove that capability from folks with a server-side filter is everything! Cheers!)